https://searchfox.org/mozilla-central/rev/68b2e0fd4323261a229233ec2ab8606228979141/intl/locale/LocaleService.cpp#356-357
!result is twice in the "if "

(In reply to Sylvestre Ledru [:Sylvestre] from comment #0)

https://searchfox.org/mozilla-central/rev/68b2e0fd4323261a229233ec2ab8606228979141/intl/locale/LocaleService.cpp#356-357
!result is twice in the "if "

That's because the variable is passed to expressions before appearing in the conditional expression. That, plus short circuiting, means this code is trying to check whether two function calls both succeeded as far as I can tell.

hmm, is that an actual bug? Should I separate them?

Ehsan explained how I was wrong.
IMHO, it should be separated to make it more obvious but it is indeed a P5 :)

Should I split the single if condition into two like this:
if (NS_FAILED(path->IsFile(&result)) || !result) {
return false;
}
if (NS_FAILED(path->IsReadable(&result)) || !result) {
return false;
}
And maybe also add comment to make the code more understandable?

Zibi is a better person for this!

Other question, what happens if we just remove those checks and see if we can read the file?

(In reply to Sylvestre Ledru [:Sylvestre] from comment #5)

Zibi is a better person for this!

Sorry, I just thought that maybe I'm expected to work on this bug because Assignee was not changed.

Sorry, I just thought that maybe I'm expected to work on this bug because Assignee was not changed.

If you want to work on that, I'd be very happy to accept your contribution! Thank you!

Other question, what happens if we just remove those checks and see if we can read the file?

:jfkthame - you wrote the original function. What do you think about Axel's suggestion?

So this is clearly a flaw in the clang analysis, not a bug in the code: as Ehsan observed, the result variable is used to return an outparam result from two successive function calls, and tested after each one. So we're not checking the same value twice, we're checking two entirely separate values. C++ rules for evaluation of logical operators give us the proper order of execution, so that each function is called and its nsresult return code checked for success before the actual result value is examined; the second check isn't simply a duplicate of the first.

Having said that, I guess it would be pretty safe to drop at least the second check (for IsReadable) here, given that if the file isn't readable, we're going to fail the following Open or fread call. I'd be reluctant to drop the check for IsFile, as I don't know exactly what the subsequent calls would do if the given name exists but is not a file (e.g. it's a directory) - presumably something would fail, and we'd end up returning false, but it seems better to do the clear, simple check early and bail out if the build is so broken that the expected file isn't present.